fix(electron): block untrusted window.open and navigations#74
Conversation
Closes #45 and #46. Add a hardenWindow helper that wires setWindowOpenHandler / will-navigate / will-redirect on every BrowserWindow so that only URLs accepted by assertTrustedUrl can load inside the app. Untrusted window.open targets are delegated to shell.openExternal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b72719e67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isTrustedUrl(url)) { | ||
| return { action: "allow" } | ||
| } |
There was a problem hiding this comment.
This allow branch trusts any URL whose origin is "null", which includes schemes like data: and javascript: (not just your app files). Because isTrustedUrl() delegates to assertTrustedUrl(), a renderer can still open opaque-origin popup URLs inside Electron instead of being denied/externalized, which bypasses the new hardening goal for untrusted window.open targets.
Useful? React with 👍 / 👎.
| win.webContents.setWindowOpenHandler(({ url }) => { | ||
| if (isTrustedUrl(url)) { | ||
| return { action: "allow" } | ||
| } |
There was a problem hiding this comment.
Returning { action: "allow" } lets Electron create a new BrowserWindow, but this code never attaches hardenWindow() to that child window. As a result, any allowed popup (trusted initial URL) can later navigate/open untrusted URLs without the will-navigate / will-redirect / setWindowOpenHandler guards applied here, leaving a bypass path for the new protections.
Useful? React with 👍 / 👎.
| if (isTrustedUrl(url)) { | ||
| return { action: "allow" } | ||
| } | ||
| void shell.openExternal(url).catch(() => { |
There was a problem hiding this comment.
This forwards every untrusted popup URL directly into shell.openExternal, including attacker-controlled custom protocols. Electron’s security guidance warns this can be abused to invoke arbitrary external handlers/apps, so a compromised renderer (e.g., XSS in any trusted page) can escalate impact from “blocked in-app popup” to launching host-level protocol handlers unless you explicitly restrict allowed schemes (typically https/http, optionally mailto).
Useful? React with 👍 / 👎.
| if (isTrustedUrl(url)) { | ||
| return { action: "allow" } | ||
| } |
There was a problem hiding this comment.
The trust predicate returns true for any file: URL, and this handler now uses that predicate to allow popup creation. That means a renderer can open arbitrary local files (e.g. file:///tmp/...) inside the app instead of being denied/externalized, which undermines the “untrusted window.open” hardening in this commit and expands the set of in-app destinations far beyond your packaged app content.
Useful? React with 👍 / 👎.
1 participant
Summary
hardenWindowhelper that wiressetWindowOpenHandler,will-navigate, andwill-redirecton everyBrowserWindow.window.opentargets are delegated toshell.openExternalso they leave the app instead of being loaded with the preload bridge attached.Closes #45 and #46.
Test plan
npm run typechecknpm run lintnpm run dev).🤖 Generated with Claude Code